fix(github-webhook): /ok-to-test is not triggering CI on PRs#2682
fix(github-webhook): /ok-to-test is not triggering CI on PRs#2682zakisk wants to merge 2 commits intotektoncd:mainfrom
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2682 +/- ##
==========================================
- Coverage 58.99% 58.96% -0.04%
==========================================
Files 207 207
Lines 20363 20378 +15
==========================================
+ Hits 12014 12015 +1
- Misses 7578 7591 +13
- Partials 771 772 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by moving authentication and secret management into dedicated gitclient and secrets packages, and relocates GitHub App token scoping logic to the GitHub provider. It also improves issue comment event handling by ensuring the client is initialized correctly. Feedback highlights a potential data race and unnecessary error logging when fetching the global repository, as well as a bug where trimming the webhook payload could break HMAC signature validation.
|
reviewing this today, can you create and link a jira to it please? we want to track it in changelog downstream |
yeah added I thought that I added it before |
884b7ab to
d7c9dd6
Compare
this moves the git provider client setup logic to its own package gitclient so that we can call it from github package. at the moment if we call client setup from github it creates import cycle. Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
When an unauthorized user opens a pull request on a repository
configured with Pipelines-as-Code using GitHub webhook
integration, commenting /ok-to-test as an admin does not trigger
the CI pipeline. This happens because the GitHub client (ghClient)
is never initialized for webhook-based issue comment events — the
client setup only ran for GitHub App events during payload parsing.
Root cause:
- In the issue_comment handler, the code checked if ghClient was
nil and returned an error, but for webhook integrations the
client is legitimately nil at that point since webhooks
authenticate differently from GitHub Apps.
- The PR number was being extracted by parsing the HTML URL string
instead of reading it directly from the event object.
- The webhook request payload and headers were not being preserved
on the event object, which is needed for webhook signature
validation.
Changes:
- pkg/provider/github/parse_payload.go:
- Add initGitHubWebhookClient() to initialize the provider
client for webhook-based events using
gitclient.SetupAuthenticatedClient
- Preserve request headers and payload on the event object
early in ParsePayload so they are available for webhook
signature validation
- Reorder handleIssueCommentEvent to match the repository
first, then lazily initialize the GitHub client if nil
(webhook case), before fetching the pull request details
- Use event.GetIssue().GetNumber() directly instead of parsing
the PR number from the HTML URL string
- Remove the early ghClient nil check that blocked webhook
events
- pkg/provider/github/github.go:
- Move GitHub App token scoping logic from gitclient into
SetClient, keeping provider-specific concerns within the
provider package
- pkg/gitclient/client_setup.go:
- Remove GitHub App token scoping (moved to provider)
- Add global repository lookup when globalRepo is nil, so
webhook-based flows can resolve credentials from the global
repository configuration
- Replace github provider import with metav1 for the Get call
- pkg/provider/github/parse_payload_test.go:
- Remove test cases that asserted ghClient nil was an error
(no longer applicable)
- Remove test for invalid PR URL parsing (PR number now read
from event)
- Add Number field to IssueCommentEvent test fixtures
- pkg/provider/github/acl_test.go:
- Add html_url and number to issue comment test payload to
match new handleIssueCommentEvent flow that sets URL and
PR number from the event object
- pkg/provider/github/github_test.go:
- Add Logger, pacInfo, and repo with Settings to SetClient
test to support token scoping moved into SetClient
- pkg/gitclient/client_setup_test.go:
- Add GlobalRepository and Namespace to test seed data to
match new global repo lookup
- pkg/pipelineascode/pipelineascode_test.go:
- Add GlobalRepository and Kube namespace to Run.Info to
match new global repo lookup in SetupAuthenticatedClient
- pkg/reconciler/reconciler_test.go:
- Add Logger to Provider in reconciler test to support
token scoping logging in SetClient
Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
d7c9dd6 to
76bb854
Compare
📝 Description of the Change
When an unauthorized user opens a pull request on a repository
configured with Pipelines-as-Code using GitHub webhook
integration, commenting /ok-to-test as an admin does not trigger
the CI pipeline. This happens because the GitHub client (ghClient)
is never initialized for webhook-based issue comment events — the
client setup only ran for GitHub App events during payload parsing.
Root cause:
nil and returned an error, but for webhook integrations the
client is legitimately nil at that point since webhooks
authenticate differently from GitHub Apps.
instead of reading it directly from the event object.
on the event object, which is needed for webhook signature
validation.
Changes:
pkg/provider/github/parse_payload.go:
client for webhook-based events using
gitclient.SetupAuthenticatedClient
early in ParsePayload so they are available for webhook
signature validation
first, then lazily initialize the GitHub client if nil
(webhook case), before fetching the pull request details
the PR number from the HTML URL string
events
pkg/provider/github/github.go:
SetClient, keeping provider-specific concerns within the
provider package
pkg/gitclient/client_setup.go:
webhook-based flows can resolve credentials from the global
repository configuration
pkg/provider/github/parse_payload_test.go:
(no longer applicable)
from event)
pkg/provider/github/acl_test.go:
match new handleIssueCommentEvent flow that sets URL and
PR number from the event object
pkg/provider/github/github_test.go:
test to support token scoping moved into SetClient
pkg/gitclient/client_setup_test.go:
match new global repo lookup
pkg/pipelineascode/pipelineascode_test.go:
match new global repo lookup in SetupAuthenticatedClient
pkg/reconciler/reconciler_test.go:
token scoping logging in SetClient
JIRA
https://redhat.atlassian.net/browse/SRVKP-11557
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.